Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v5] Check paths in Query.isEqual #2738

Closed
wants to merge 1 commit into from

Conversation

iwikal
Copy link
Contributor

@iwikal iwikal commented Oct 16, 2019

Summary

Query.isEqual should return false when you compare two queries on different collections.

Checklist

  • Supports Android
  • Supports iOS
  • e2e tests added or updated in packages/**/e2e
  • Flow types updated
  • Typescript types updated

Test Plan

The plan is to wait and see if CI accepts the PR.

Release Plan

[JS][BUGFIX] [firestore] - Query.isEqual now takes the collection path into consideration


Think react-native-firebase is great? Please consider supporting the project with any of the below:

@iwikal iwikal changed the title Check paths in Query.isEqual [v5] Check paths in Query.isEqual Oct 16, 2019
@mikehardy
Copy link
Collaborator

@Ehesp / @Salakar what do you guys think of this? If it's an issue, it's an issue on v6 as well: https://github.com/invertase/react-native-firebase/blob/master/packages/firestore/lib/FirestoreQuery.js#L154 - no path comparison there if I'm reading it correctly

@iwikal
Copy link
Contributor Author

iwikal commented Oct 16, 2019

I assume you want the same functionality as in the web sdk, where path does matter.

@iwikal
Copy link
Contributor Author

iwikal commented Oct 16, 2019

I can't really make sense of the build errors; something about a timeout in beforeAll? I couldn't build an android apk to test locally on my linux machine either.

@mikehardy
Copy link
Collaborator

The ci.android.test build right now is non-functional for unrelated reasons - whether your change is correct or not

ci.ios.test should be passing though, there's a separate PR (#2726) that is also experiencing failures on CI there while it works local, and we're not sure why. We are investigating

@mikehardy
Copy link
Collaborator

@Salakar @Ehesp I think this is also an issue on v6 but the line has moved it is here now https://github.com/invertase/react-native-firebase/blob/master/packages/firestore/lib/FirestoreQuery.js#L176 (although it's missing, so it's not there - there is no path check)

The underlying assumption that we should mirror web and check for path also seems valid, so I think this should be merged? And v6 should get the same treatment?

@mikehardy mikehardy added Workflow: Needs Review Pending feedback or review from a maintainer. Version: 6.x.x plugin: firestore Firebase Cloud Firestore labels Dec 12, 2019
@iwikal
Copy link
Contributor Author

iwikal commented Dec 16, 2019

It seems that the web sdk implements CollectionReference as a subclass of Query. It follows that if the Query.isEqual method compares paths, CollectionReference.isEqual can be straight up inherited from that. Maybe it's something we don't want to do for performance reasons, but it would be correct.

@russellwheatley
Copy link
Member

This was merged into v6: #3738, but will not be getting merged into v5 as it is no longer supported. Thank you for bringing this to our attention @iwikal, much appreciated.

@mikehardy mikehardy removed the Workflow: Needs Review Pending feedback or review from a maintainer. label Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin: firestore Firebase Cloud Firestore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants